Render SSAT winning creative inline without PBS Cache round trip#899
Render SSAT winning creative inline without PBS Cache round trip#899prk-Jr wants to merge 25 commits into
Conversation
- Drop render_adm param: always include adm when creative present
- Gate GAM-bypass on per-bid debug_bid instead of a global window.tsjs flag
(removes SPA-staleness edge case, TsjsApi change, and whole flag-emit task)
- Correct fallback scope: cache fallback only when adm absent; render failure
after adm is supplied is not detectable
- Qualify sandbox claim: TS guarantees script-context escaping; bridge frame
isolation depends on the Prebid Universal Creative
- Reconcile with existing ad_init.test.ts coverage (rename, no duplicates)
- Fix test assertion messages to expect("should ..."); list exact clippy gates
- Task 3: assert observable DOM (GAM iframe src) instead of spying on the module-private injectAdmIntoSlot - Use existing make_bid helper + set .creative (make_test_bid_with_creative does not exist on this branch) - Hostile-adm regression test covers both U+2028 and U+2029 - Add 'cd docs && npm run format' to verification
build_bid_map now always inserts the winning creative as adm so the pbRender bridge can render it locally (no PBS Cache round trip); the verbose debug_bid blob and the GAM-bypass gate stay behind inject_adm_for_testing. Rename the param include_adm -> include_debug_bid and thread it through write_bids_to_state. Reconcile the by-default test to the new behavior, drop the now-redundant debug-only-adm test, and pin script-context escaping for a hostile adm (</script> + U+2028/U+2029).
The direct GAM-replace path (injectAdmIntoSlot) now fires only when the bid carries debug_bid, which is present only under inject_adm_for_testing. In production the always-present adm is rendered by the pbRender bridge and GAM stays in the loop. Add observable-DOM tests (GAM iframe src unchanged without debug_bid, rewritten with it), strengthen the bridge test to prove inline adm is preferred even when cache coords are present, and rename debug-adm terminology to inline adm.
aram356
left a comment
There was a problem hiding this comment.
Review summary
The inline adm path is a useful latency improvement, and the per-bid debug gate is a good way to avoid stale SPA-global state. I am requesting changes for two production contract gaps before this ships:
- The new SSAT path bypasses the existing creative sanitization, URL-rewrite, and size-limit boundary.
- The advertised PBS Cache fallback forwards a cached bid JSON document as creative markup instead of extracting its
adm.
Additional non-blocking finding
♻️ Update the public contracts to match the new semantics. Several comments outside this diff still say adm is present only under inject_adm_for_testing: Settings::inject_adm_for_testing, AuctionBidData.adm, and the GPT integration comments at lines 279–285 and 825–835. Please update them to distinguish production inline adm from the testing-only direct GAM replacement gated by debug_bid.
Verification
- Reviewed all five changed files against base
0e5dbb211da5e61598681df3abfda239484afa5f. - Current GitHub CI is green (19 reported checks).
- Focused GPT Vitest suite: 26/26 passing.
- Focused Rust regressions: 2/2 passing.
git diff --check: clean.- Local
cargo test-fastlycompiled, but Viceroy could not access the macOS certificate keychain (No keychain is available); the corresponding GitHub Fastly job passes, so this is a local environment limitation rather than a PR failure.
…code cached bids The inline-adm path fed raw bid.creative into window.tsjs.bids, bypassing the creative-processing boundary the /auction path applies (sanitize_creative_html then rewrite_creative_html, which also enforces the 1 MiB creative cap). Run the same boundary in build_bid_map before inserting adm; omit adm when the creative is rejected so the bridge falls back to the PBS Cache coordinates. Thread &Settings through build_bid_map and write_bids_to_state for rewrite_creative_html. The pbRender bridge's PBS Cache fallback forwarded the raw GET body as the ad, but PBS Cache returns a JSON bid object and the Prebid Universal Creative renders bidObject.adm. Add extractCachedAdm to parse the cached bid and extract its adm, keep raw-markup compatibility, and decline to render when no adm is present. Add hostile and oversized creative coverage in Rust, and realistic returnCreative=false, malformed, and raw-markup cache-response coverage in JS. Update the inject_adm_for_testing, AuctionBidData, GPT bridge, and design-spec docs to distinguish production inline adm from the testing-only debug_bid path.
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed against base 0e5dbb211, focusing on 1c983027c and the two blocking findings from the last round.
The PBS Cache decode finding is resolved: extractCachedAdm mirrors PUC's bidObject.adm extraction, keeps raw-markup compatibility, declines on malformed payloads, and the new tests use realistic returnCreative=false shapes. The doc updates also resolve the earlier contract-drift finding.
The sanitize/rewrite finding is only half resolved. Sanitization and the 1 MiB cap are correctly applied, but rewrite_creative_html was designed for creatives rendered from the first-party origin, and reusing it here rewrites the inline adm into root-relative URLs that will not resolve in the context the pbRender bridge hands them to. Details inline on publisher.rs:2003.
Blocking
🔧 wrench
- Rewritten inline
admemits root-relative URLs that will not resolve in PUC's render context (crates/trusted-server-core/src/publisher.rs:2003)
Non-blocking
🤔 thinking
- The rewriter injects the full tsjs bundle into every creative (
crates/trusted-server-core/src/publisher.rs:2002) - Declining after
stopImmediatePropagation()leaves the slot blank (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:967) - The 1 MiB cap is per-creative, not aggregate.
write_bids_to_stateinlines the creative for every winning slot into the page's<script>on every page load, so N slots can inline up to N MiB with no aggregate bound. Per this PR's own test plan, GAM usually awards its own demand and the bridge never fires — in that common case the inlined bytes are pure document-weight/TTFB cost. Worth considering an aggregate cap, or a narrower condition for inlining.
♻️ refactor
- No test covers the rewrite half of the boundary (
crates/trusted-server-core/src/publisher.rs:3967)
📌 out of scope
- Please do file the PUC tracking issue you offered. Deferring the PUC version/template pin was reasonable on its own terms, but the blocking finding above makes it load-bearing: the render context that pin would establish is exactly what determines whether the inline
adm's rewritten URLs resolve. Confirming the GAM-served PUC bundle and adding a browser contract test against it would settle both that finding and the earlierrendererVersion/renderer-field question.
CI Status
- fmt: PASS (GitHub)
- clippy: PASS (GitHub)
- rust tests: PASS (GitHub;
cargo test, axum native, cloudflare, spin, parity, CLI all green). Localpublisher::tests84/84 pass. - js tests: PASS (GitHub vitest; local GPT suite 77/77 pass)
- All 19 reported checks are green.
The inline `adm` in window.tsjs.bids is rendered by the Prebid Universal Creative inside GAM's iframe (f.srcdoc = d.ad), a foreign origin. rewrite_creative_html emitted root-relative /first-party/... URLs that resolve against GAM's origin there and 404, and injected the tsjs bundle into every creative iframe. Add rewrite_inline_creative_html: emits absolute https://<publisher.domain>/first-party/... URLs (resolve regardless of the document base URL or whether PUC honors the custom renderer) and skips the tsjs bundle injection. Route build_bid_map's inline adm through it; the first-party /auction path keeps the root-relative rewriter. One shared rewriter via base_origin/inject_tsjs. Add tests for the absolute-URL shape and absent tsjs injection at the creative and build_bid_map boundaries.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed the current head after removing the earlier first-party iframe-proxy blocker. The previous raw-creative, cache-JSON, root-relative URL, and unwanted TSJS-injection findings are resolved. CI is green, but the new default inline-render path still has three high-confidence compatibility/correctness gaps, plus one cache-fallback contract gap and stale design records. Details are inline.
hb_adid is not unique per bid: absent PBS Cache it falls back to a creative id a bidder may reuse across slots (observed: three IX bids sharing one id). The bridge matched the first bid whose hb_adid equalled the requested adId, then rejected on the slot-ownership guard, so every slot but the first rendered blank. Resolve the bid by the requesting slot and verify its hb_adid matches the request, so each slot renders its own creative regardless of duplicate ids. The adId check still blocks a slot A iframe from pulling slot B's creative and beacons.
The pbRender bridge sized every inline response from the first configured slot format, while the winning creative's own width/height were emitted only inside the testing-only debug_bid. A multi-size slot whose winner is not the first format therefore rendered at the wrong size (clipping or whitespace). Emit w/h in the normal bid map and AuctionBidData, and prefer them in the inline bridge response, falling back to the first slot format only when absent.
The inline render path forwarded adm without resolving the auction-price
macro. URL rewriting then serialized query pairs, encoding the literal
${AUCTION_PRICE} into %24%7BAUCTION_PRICE%7D inside the signed proxy/click
URL — so trackers received an encoded macro rather than the clearing
price, and signing locked the wrong value.
Add expand_auction_price_macro and call it from build_bid_map before
sanitize_creative_html and rewrite_inline_creative_html, using the exact
winning CPM. Only the clear-price token is expanded; the encrypted
${AUCTION_PRICE:B64} variant is left for the DSP.
rewrite_inline_creative_html hard-coded https://{publisher.domain},
discarding the incoming scheme, subdomain, and port. publisher.domain
cannot carry a port, deployments may serve a subdomain, and Axum/Viceroy
dev runs over HTTP with a port — so inline proxy/click URLs resolved
against the wrong origin with no render-time fallback.
Thread the trusted request origin (scheme://host, host including any
port) through write_bids_to_state and build_bid_map into
rewrite_inline_creative_html. Initial navigation derives it from the
buffered request host/scheme; SPA page-bids from RequestInfo. Falls back
to the configured publisher domain only when the origin is unknown.
Covers HTTP localhost with a port, a request subdomain differing from
publisher.domain, and a non-default HTTPS port.
extractCachedAdm reduced the cached bid to its adm string, so the cache
fallback sized every render from the first slot format and left price
macros unresolved. Replace it with parseCachedBid, which retains the
cached creative dimensions (w/h or width/height) and clearing price.
The fallback now sizes from the cached dimensions (slot format only when
absent) and expands ${AUCTION_PRICE} from the cached price before
responding. Raw-markup bodies stay supported as the adm-only variant.
Firing a cached win-notification URL is deferred: it is a billing side
effect and the exact cache field/dedup contract needs a real PBS Cache
payload to verify before emitting.
The design named rewrite_creative_html and omitted the render-metadata
requirements the code now enforces; the plan's tasks were unchecked and
predated the shipped divergences. Add an "Implementation reconciliation"
section to the design (inline rewriter, request-origin URLs, w/h and
${AUCTION_PRICE} render metadata, structured cache decode) and update the
components table; mark the plan superseded-but-completed pointing at it.
Threading the request origin pushed the private collect_stream_auction helper to 8 arguments. Its arguments mirror the AuctionCollectCtx fields the caller destructures, so a parameter struct would only duplicate that context; suppress the lint instead.
Missing OpenRTB w/h parse to 0, and build_bid_map emitted them as w:0/h:0. The bridge nullish-coalesces (matchedBid.w ?? fallback), so 0 was kept rather than falling back to the slot format — sizing the frame to 0. Omit a non-positive dimension server-side, and treat a zero cached dimension as absent in parseCachedBid, so the bridge falls back.
The cache-fallback concurrency guard keyed on adId (hb_adid), which is not unique per bid — two distinct slots sharing an hb_adid would have the second's render cross-blocked. Not reachable today (a duplicate hb_adid means no cache coordinates, so the cache path never runs), but the guard relied on that invariant silently. Key the in-flight set on slotId|adId so it dedups a genuine same-render race without colliding across slots.
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at f4e0a79fa against base 0f8f93921, focusing on the work since 1c983027c.
Both of my earlier blocking findings are resolved, and the four raised in the interim review land correctly. One new blocking issue: this PR introduces ${AUCTION_PRICE} expansion for the creative but not for the nurl/burl notification URLs, which is where that macro most commonly appears — and this PR is what promotes those beacons from a testing-only path to the primary production one.
Blocking
🔧 wrench
nurl/burlfire with an unexpanded${AUCTION_PRICE}(crates/trusted-server-core/src/publisher.rs:2183-2188, expansion at:2211)
Non-blocking
♻️ refactor
#[allow(clippy::too_many_arguments)]sidesteps a struct that already exists (crates/trusted-server-core/src/publisher.rs:1383)
📝 note
- Earlier findings verified as resolved.
rewrite_inline_creative_htmlgenuinely fixes the foreign-origin URL problem. I checked the part that could have silently broken:build_signed_url_forcomputes the token overtsurl+ params only and excludesbase_path, so absolute inline URLs validate through the proxy identically to root-relative ones.base_originderives fromRequestInfo.host, already the established origin source for URL rewriting across the codebase, so it is not a new input surface. The dimension, origin-threading, macro-ordering, and cache-decode fixes all check out, and the slot-scoped bid lookup correctly resolves the duplicate-hb_adidcollision. Thanks for filing #926 with the PUC pin, the third-party-cookie check, and the aggregate-admcap.
CI Status
- fmt: PASS (GitHub)
- clippy: PASS (GitHub)
- rust tests: PASS — 19/19 GitHub checks green; local
trusted-server-corelib suite 1659/1659 pass - js tests: PASS — local full Vitest run 425/425 pass across 30 files
Per OpenRTB, nurl and burl are the canonical carriers of the
${AUCTION_PRICE} macro, and the render bridge fires both verbatim via
sendBeacon. Only the creative was expanded, so every TS-won impression
sent the SSP a literal macro instead of the clearing price — a
revenue-reporting error, and the kind of malformed notification some
SSPs reject outright. Expand both from the same winning CPM already in
scope for the creative.
The PBS Cache fallback in the bridge is covered by the same change: it
fires matchedBid's beacons, which now arrive expanded from the auction
clearing price rather than the cached copy's price. Note that at the
fire site so the expansion is not duplicated client-side.
The debug_bid mirror keeps its bidder-supplied creative, nurl, and burl
verbatim: it is diagnostic, nothing renders or fires from it, and
showing what the bidder actually sent is the point.
Also drop the too_many_arguments allow on collect_stream_auction. Split
AuctionCollectCtx into the per-auction state that is moved at collect
time and the borrowed dependencies that outlive it, so the helper takes
three arguments and request-origin threading no longer widens the
signature.
* Suppress fabricated empty Prebid bidder params
A configured bidder with no inline params and no matching override
expanded to `"bidder": {}`, which PBS rejects. After applying overrides,
drop fabricated empty bidders, preserve an explicitly supplied empty
object so genuine misconfiguration stays visible, and fall back to the
stored-request path when no eligible bidders remain.
* Prefer direct bidder params over fabricated-empty in mixed slots
A slot can carry both a direct bidder entry with valid inline params and a
trustedServer entry whose bidderParams omits that bidder. Because slot.bidders
is a HashMap, the trustedServer expansion (which fabricates an empty {} for the
omitted bidder) could run after the direct entry and overwrite its valid params
via extend. The direct entry already marked the bidder explicit, so the empty
object survived the retention drop and PBS rejected the impression — the failure
this path is meant to eliminate — nondeterministically, depending on iteration
order.
Replace the extend with an entry().or_insert() loop so trustedServer-expanded
bidders only fill in absent entries and direct params win regardless of order.
Add a looped regression test exercising a slot with both representations.
* Drop empty and non-object PBS bidder params instead of shipping them
expand_trusted_server_bidders and the direct merge could both put an
unusable value into imp.ext.prebid.bidder — an empty {} (fabricated or
publisher-supplied) or a non-object like null. PBS rejects such an imp, and
parse_response collapses one non-2xx into an auction-wide bid wipeout, so a
single misconfigured slot zeroed every slot's bids.
- Add is_unusable_bidder_params (empty object or non-object).
- Collect the trustedServer expansion and direct entries separately, then
merge so a direct entry wins but an unusable value never clobbers real
params from the expansion (fixes the asymmetric direct-{} overwrite).
- Drop every remaining unusable value after overrides and log::warn each
drop; the slot then falls back to its stored request. Explicit empties are
no longer preserved — PBS cannot tell them from fabricated ones.
- Correct the stored-request comment: the fallback also fires for real
inline params naming a bidder absent from config.bidders.
- Tests: explicit-empty now drops to stored request; add direct-empty
no-clobber, null drop, and unconfigured-bidder fallback.
* Split unusable-bidder drop logging by source and collapse per slot
The unconditional drop of empty/non-object PBS bidder params warned once
per bidder, including on the designed creative-opportunity path where the
trustedServer expansion fabricates empty params and no override rule fills
them. Routine slots emitted N warnings describing intended behavior, burying
the genuine publisher misconfiguration the log exists to surface.
Track which bidders the slot supplied inline and split the drop log by
source: slot-supplied unusable params warn, fabricated empties log at debug.
Both now emit one line per slot with sorted bidder names instead of one line
per bidder. Drop behavior is unchanged.
Summary
admis now always included in
window.tsjs.bids, so the existing render bridgeserves it locally when GAM's Prebid line item fires — eliminating the
render-time PBS Cache round trip and matching how client-side
/auctionrenders from the copy the browser already holds.
hb_pb, andhb_cache_host/hb_cache_pathremain as the fallback for an absentadm.No GAM bypass in production.
debug_bidblob stays behindinject_adm_for_testing. TheGAM-bypass path (
injectAdmIntoSlot) is now gated on the per-biddebug_bidfield instead of
bid.adm— no new global flag, noTsjsApichange, correctacross SPA auction responses.
Changes
crates/trusted-server-core/src/publisher.rsbuild_bid_mapalways insertsadmwhenbid.creativeisSome; paraminclude_admrenamed toinclude_debug_bid(now gates only thedebug_bidblob);write_bids_to_stateparam renamed to match. Added a hostile-admregression test proving</script>/U+2028/U+2029cannot break out of the emitted<script>.crates/trusted-server-js/lib/src/integrations/gpt/index.tsbid.adm && bid.debug_bid; render-bridge log message updated to "from inline adm".crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.tsdebug_bidis present); renamed "debug adm" terminology to "inline/local adm"; bridge fixtures now carry bothhb_cache_*coords and inlineadm, proving the bridge prefers localadm.docs/superpowers/specs/2026-07-13-ssat-render-inline-creative-design.mddocs/superpowers/plans/2026-07-13-ssat-render-inline-creative.mdCloses
Closes #898
Test plan
cargo test-fastly && cargo test-axum(alsocargo test-cloudflare && cargo test-spin— all green)cargo clippy-fastly && cargo clippy-axum(onlyclippy-fastlyrun locally; full matrix delegated to CI)cargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run(407 passed)cd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1(delegated to CI)fastly compute serveadmnow present inwindow.tsjs.bidsfor all slots. The full inline-render path (GAM awards the Prebid line item → creative renders with no PBS Cache fetch) is not yet exercised locally — mocktioneer's $0.20 bid loses to GAM's own demand, so GAM serves its own creative and the bridge never fires. Requires a winning Prebid line item in GAM to verify end-to-end.Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!) — no new Rust logging introduced